-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(metrics): add internal rpc metrics #618
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
|
use serde::{de::DeserializeOwned, Deserialize, Serialize}; | ||
use tokio::time::Instant; | ||
|
||
#[derive(Display, Deserialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need Display
? Confused why we need serde and display on these enums, which one does the metrics library use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im pretty sure the Display macro still needs to be there if we want the auto to_string implemented without having to manually implement fmt::Display for Enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what is serde used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serde is For Serializing and Deserializing back and for from rust data types to usable structures like json and yaml. But from what I understand if you want to_string you need to implement Display. The reason we need both here is because the container attribute need to access the Deserializer from the derive. Well at least thats what I think but when I remove it, it gives an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to remove both Deserialize
and #[serde(...)]
, I don't see that being used anywhere. Display
is being used for to_string()
and you can add back that #[display(style = "lowercase")]
if you want to lowercase (maybe use snake case?) the enums
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh that its true, I have just changed
99f4e84
to
c594b51
Compare
c594b51
to
a545d7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This will be huge for observability
Closes #588
Proposed Changes
provider/ethers
module